Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Forward ref #307

Merged
merged 12 commits into from
May 25, 2021
Merged

Forward ref #307

merged 12 commits into from
May 25, 2021

Conversation

ZoteTheMighty
Copy link
Contributor

@ZoteTheMighty ZoteTheMighty commented May 25, 2021

This closes a gap in compatibility with upstream React. While the current Roact implementation doesn't support refs assigned to non-host components, it also doesn't provide a way for non-host components to forward refs idiomatically as described here: https://reactjs.org/docs/forwarding-refs.html

This change introduces an upstream-compatible forwardRef API and loosely adapts some of the upstream tests as well.

Checklist before submitting:

  • Add a test to validate that the ref isn't also provided as a member of props
  • Added entry to CHANGELOG.md
  • Added/updated relevant tests
  • Added/updated documentation

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 94.244% when pulling 16587b3 on forwardRef into 8312f84 on master.

@ZoteTheMighty ZoteTheMighty marked this pull request as ready for review May 25, 2021 02:40
Copy link
Contributor

@oltrep oltrep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@matthargett matthargett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely need to include the copyright and license info at the top of the derives test file. other than that, LGTM.

@@ -0,0 +1,341 @@
-- Tests loosely adapted from those found at:
-- * https://github.com/facebook/react/blob/v17.0.1/packages/react/src/__tests__/forwardRef-test.js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put the LICENSE reference. and copyright information here as well

@ZoteTheMighty ZoteTheMighty merged commit 058e3c6 into master May 25, 2021
@github-actions github-actions bot locked and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants